Always adding :debug_info on erlc_options #9348

kelvinst · 2019-09-14T13:37:20Z

That way, if someone sets erlc_options to [warnings_as_errors: true], :debug_info would still be on the options. To configure it to not save debug info, one should only add debug_info: false to erlc_options

Closes #9184

That way, if someone sets erlc_options to `[warnings_as_errors: true]`, `:debug_info` would still be on the options. To configure it to not save debug info, one should only add `debug_info: false` to erlc_options

kelvinst · 2019-09-14T13:38:23Z

I also fixed a doc that was pointing to the wrong default value for :debug_info option on Code.put_compiler_option

kelvinst · 2019-09-14T13:42:45Z

I'm checking how to add tests for this now.

Also, there should be a changelog notice or something like that, since the change breaks compatibility. e.g. if anyone was just dismissing :debug_info on :erlc_options to disable it, that would not work anymore, they would have to change their options to debug_info: false

kelvinst · 2019-09-14T14:00:50Z

I'm checking how to add tests for this now.

So as it is done now, I'm not sure there is any good way of testing it, since it is an inner logic inside Mix.Tasks.Compile.Erlang.do_run. Should I extract the erlc_options build to a public function and unit test it? Maybe we can extract it to Mix.Compilers.Erlang.erlc_options and even document it better there. WDYT?

josevalim · 2019-09-14T15:24:58Z

You should be able to test it like this: https://github.com/elixir-lang/elixir/blob/master/lib/mix/test/mix/tasks/compile.erlang_test.exs#L14

kelvinst · 2019-09-14T19:18:41Z

Yeah, I saw that, but what I couldn't figure out yet is how to check if the :debug_info option was actually applied to compilation.

michalmuskala · 2019-09-14T19:37:48Z

@kelvinst it should be possible to check the generated files with :beam_lib.chunks(path, [:debug_info])

kelvinst · 2019-09-14T20:42:55Z

Cool! Thanks @michalmuskala! Really helpful tip (I suppose you mean :beam_lib.chunks, right?)

kelvinst · 2019-09-14T21:22:10Z

PS.: I just realized that I added the default options to the wrong place I guess, that way it will apply it only for the mix compile.erlang task, not when using mix compile. Will fix later.

kelvinst · 2019-09-15T15:41:00Z

Actually, nevermind, just realized mix compile does call mix compile.erlang 🤦‍♂ I'm investigating why @hauleth's example app still don't work with my solution.

kelvinst · 2019-09-15T17:42:53Z

Right, so the problem was that I was just running ../elixir/bin/mix do compile, dialyzer from inside @hauleth's project, and that was not using ../elixir/bin/mix to run the subtasks I suppose. Did test setting PATH=../elixir/bin:$PATH mix do compile, dialyzer and it did work that way, so that really worked.

michalmuskala · 2019-09-15T18:07:52Z

lib/mix/test/mix/tasks/compile.erlang_test.exs

+      {:ok, {module, [debug_info: {:debug_info_v1, backend, data}]}} =
+        :beam_lib.chunks(binary, [:debug_info])
+
+      assert backend.debug_info(:elixir_v1, module, data, []) != {:error, :missing}


I think this should assert with :erlang_v1 - after all it's compiling erlang modules, so it's kind of expected it won't be able to return elixir debug info.

Suggested change

assert backend.debug_info(:elixir_v1, module, data, []) != {:error, :missing}

assert backend.debug_info(:erlang_v1, module, data, []) != {:error, :missing}

I wouldn't even assert this. Asserting it returns a debug info chunk is probably good enough!

You mean something like this?

{:ok, {_, [debug_info: {:debug_info_v1, _, {info, _}}]}} = :beam_lib.chunks(binary, [:debug_info]) assert info != :none

I've checked and the only difference is that it returns :none for that part when debug info is disabled, instead of the list of attributes.

I have started with that actually, just changed to use backend.debug_info because I was afraid that maybe the format could change in the future and then the test could fail. Very unlikely, but who knows 🤷‍♂

@michalmuskala

As suggested by @michalmuskala

+ better docs for `erlc_options`

josevalim · 2019-09-15T19:23:00Z

The point is that if they change the format, this test should not fail. It is ok for them to do that. As long as there is something, we are good. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

josevalim · 2019-09-15T19:24:10Z

lib/mix/lib/mix/tasks/compile.erlang.ex

-      erlc_options: [:debug_info, {:i, 'path/to/include'}]
+  The following options are always added to `:erlc_options` when running the compiler:
+
+      [:debug_info, :return, :report, outdir: compile_path, i: include_path]


Return and report are implementation details. If the user changes them, then the compiler will fail. I would remove this section (or talk about only debug_info).

Cool, thanks!! I will comment only about debug_info.

kelvinst · 2019-09-15T19:23:20Z

lib/mix/lib/mix/project.ex

@@ -729,7 +729,7 @@ defmodule Mix.Project do
      elixirc_paths: ["lib"],
      erlc_paths: ["src"],
      erlc_include_path: "include",
-      erlc_options: [:debug_info],
+      erlc_options: [],


This can be empty since we already add it everytime when compiling erlang.

kelvinst · 2019-09-15T19:28:35Z

lib/mix/lib/mix/tasks/compile.erlang.ex

-      erlc_options: [:debug_info, {:i, 'path/to/include'}]
+  The following options are always added to `:erlc_options` when running the compiler:
+
+      [:debug_info, :return, :report, outdir: compile_path, i: include_path]


Cool, thanks!! I will comment only about debug_info.

fertapric · 2019-09-22T17:38:55Z

Thanks @kelvinst! 💜

Always adding :debug_info on erlc_options

7ee67a8

That way, if someone sets erlc_options to `[warnings_as_errors: true]`, `:debug_info` would still be on the options. To configure it to not save debug info, one should only add `debug_info: false` to erlc_options

hauleth approved these changes Sep 14, 2019

View reviewed changes

Running formatter

638fd3d

Adding tests for :debug_info on erlang compiler

bef9709

michalmuskala reviewed Sep 15, 2019

View reviewed changes

kelvinst added 2 commits September 15, 2019 15:19

Using :erlang_v1 for :erl_abstract_code.debug_info format param

5be92dd

As suggested by @michalmuskala

Set default :erlc_options to [] as now :debug_info is added

9c4eabb

+ better docs for `erlc_options`

josevalim reviewed Sep 15, 2019

View reviewed changes

kelvinst commented Sep 15, 2019

View reviewed changes

Fixing tests and docs

9f11855

josevalim approved these changes Sep 16, 2019

View reviewed changes

fertapric merged commit 87d5255 into elixir-lang:master Sep 22, 2019

al2o3cr mentioned this pull request Dec 28, 2023

Confusing / harmful directions about debug_info in the README elixir-lsp/vscode-elixir-ls#398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always adding :debug_info on erlc_options #9348

Always adding :debug_info on erlc_options #9348

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

josevalim commented Sep 14, 2019

kelvinst commented Sep 14, 2019

michalmuskala commented Sep 14, 2019 •

edited

Loading

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 15, 2019

kelvinst commented Sep 15, 2019

michalmuskala Sep 15, 2019

kelvinst Sep 15, 2019

josevalim Sep 15, 2019

kelvinst Sep 15, 2019

kelvinst Sep 15, 2019

kelvinst Sep 15, 2019

josevalim commented Sep 15, 2019 via email

josevalim Sep 15, 2019

kelvinst Sep 15, 2019

kelvinst Sep 15, 2019

kelvinst Sep 15, 2019

fertapric commented Sep 22, 2019

	assert backend.debug_info(:elixir_v1, module, data, []) != {:error, :missing}
	assert backend.debug_info(:erlang_v1, module, data, []) != {:error, :missing}

Always adding :debug_info on erlc_options #9348

Always adding :debug_info on erlc_options #9348

Conversation

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

josevalim commented Sep 14, 2019

kelvinst commented Sep 14, 2019

michalmuskala commented Sep 14, 2019 • edited Loading

kelvinst commented Sep 14, 2019

kelvinst commented Sep 14, 2019

kelvinst commented Sep 15, 2019

kelvinst commented Sep 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Sep 15, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fertapric commented Sep 22, 2019

michalmuskala commented Sep 14, 2019 •

edited

Loading